Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

split out CFDatetimeCoder, deprecate use_cftime as kwarg #9901

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

split out CFDatetimeCoder, deprecate use_cftime as kwarg #9901

wants to merge 8 commits into from

Conversation

kmuehlbauer
Copy link
Contributor

@kmuehlbauer kmuehlbauer commented Dec 17, 2024 •
edited
Loading

  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

This makes something like this possible:

import xarray as xr
from xarray.coders import CFDatetimeCoder
# available with this PR
ds = xr.open_mfdataset(..., decode_times=CFDatetimeCoder(use_cftime=True))
# available with #9618
ds = xr.open_mfdataset(..., decode_times=CFDatetimeCoder(time_unit="s"))

@kmuehlbauer kmuehlbauer marked this pull request as ready for review December 17, 2024 15:54
@kmuehlbauer kmuehlbauer mentioned this pull request Dec 17, 2024
Open
9 tasks
dcherian
dcherian reviewed Dec 17, 2024
View reviewed changes
doc/whats-new.rst Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Dec 17, 2024
View reviewed changes
xarray/backends/api.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Dec 17, 2024
View reviewed changes
xarray/backends/api.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Dec 17, 2024
View reviewed changes
xarray/conventions.py Outdated Show resolved Hide resolved
dcherian
dcherian reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this but since this is adding public API, can you open an issue and see if everyone is on board please

kmuehlbauer
kmuehlbauer commented Dec 17, 2024
View reviewed changes
doc/whats-new.rst Outdated Show resolved Hide resolved
kmuehlbauer
kmuehlbauer commented Dec 17, 2024
View reviewed changes
xarray/backends/api.py Outdated Show resolved Hide resolved
kmuehlbauer and others added 3 commits December 17, 2024 17:19
@kmuehlbauer @dcherian
Apply suggestions from code review
bd6a5d1 
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
6557ef9 
for more information, see https://pre-commit.ci
@kmuehlbauer
fix warning
759fb72
@kmuehlbauer kmuehlbauer mentioned this pull request Dec 17, 2024
Open
@kmuehlbauer
Copy link
Contributor Author

@dcherian Thanks for reviewing, addressed along your comments/suggestions. I've added a comment to #4490 for more visibility, hope that's sufficient.

@mathause
Copy link
Collaborator

Can you explicitely write how to change this in the deprecation warning? As is I don't find it obvious what users have to do. ('Use decode_times=xr.coders.CFDatetimeCoder(use_cftime=True) instead').

I am also a bit surprised that the CFDateTimeCoder does not imply use_cftime=True - should that be named DateTimeCoder? (or did I miss this discusdion?)

Is the use of DeprecationWarning intentional? That's still silent in certain situations, right?

(Removing an arg is always nice but I use this a lot and its much more verbose...)

@kmuehlbauer
Copy link
Contributor Author

kmuehlbauer commented Dec 21, 2024 •
edited
Loading

@mathause Thanks for sharing your concerns and suggestions.

Can you explicitely write how to change this in the deprecation warning? As is I don't find it obvious what users have to do. ('Use decode_times=xr.coders.CFDatetimeCoder(use_cftime=True) instead').

Yes, this is a bit sparse, how is it with:

import xarray as xr
time_coder = xr.coders.CFDatetimeCoder(use_cftime=True)
ds = xr.open_dataset(decode_times=time_coder)

I am also a bit surprised that the CFDateTimeCoder does not imply use_cftime=True - should that be named DateTimeCoder? (or did I miss this discusdion?)

So the current state of affairs is:

The default state of open_dataset kwarg decode_times depends on the backend, for most backends this is True. This will trying to decode datetimes into numpy datetime64 (pandas DatetimeIndex), if the on-disk datetimes are in the ns-representable time-range and a standard calendar is used. If this fails, cftime is used resulting in a CFTimeIndex. Setting use_cftime=True means to use the cftime-package for decoding anyway. This will result in a CFTimeIndex. In most cases (standard calendar/ proleptic gregorian) cftime is not needed to correctly decode according CF into numpy datetime64. Does that make sense?

Is the use of DeprecationWarning intentional? That's still silent in certain situations, right?

As the current behaviour is preserved, we might skip the DeprecationWarning for now until all coders have been edited to conform to some solution which originated from #4490. In the last dev meeting @TomNicholas added an item to the agenda moving the cf coders into a new package (xref https://github.com/xarray-contrib/cf-codecs and cc @rabernat). So this would be a first step into this direction.

I'll rework the DeprecationWarning to more explicit in what to do, but we should decide whether we want to warn from now or postpone this until we have a clear way of how to do things.

kmuehlbauer
kmuehlbauer commented Dec 22, 2024
View reviewed changes
xarray/conventions.py
emit_user_level_warning(
"Usage of 'use_cftime' as kwarg is deprecated. "
"Please initialize it with CFDatetimeCoder and "
"'decode_times' kwarg.",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"'decode_times' kwarg.",
"'decode_times' kwarg.\n",
"Example usage:\n",
" time_coder = xr.coders.CFDatetimeCoder(use_cftime=True)\n",
" ds = xr.open_dataset(decode_times=time_coder)\n",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathause Would that help in the DeprecationWarning?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's good.

mathause
mathause reviewed Dec 22, 2024
View reviewed changes
Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is much easier to adapt to the deprecation with the suggested message.

Does that make sense?

Yes, that makes sense - thanks!

xarray/conventions.py
emit_user_level_warning(
"Usage of 'use_cftime' as kwarg is deprecated. "
"Please initialize it with CFDatetimeCoder and "
"'decode_times' kwarg.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathause mathause mathause left review comments

@dcherian dcherian dcherian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kmuehlbauer @mathause @dcherian